Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

small fix to fontTools Documentation heading #3490

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rrawatt
Copy link
Contributor

@rrawatt rrawatt commented Apr 27, 2024

No description provided.

@rrawatt
Copy link
Contributor Author

rrawatt commented Apr 28, 2024

Changes also made to optional.rst for easy and concise readbility

@anthrotype anthrotype requested a review from n8willis May 30, 2024 11:29
Copy link
Contributor

@n8willis n8willis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

I see a couple of things needing further fixing, in optional.rst, and as long as there is some real fixing, I decided to also note a few stylistic questions. A couple are just questions.

set of keywords that describe a group of additional dependencies, which can be
used when installing via pip, or when specifying a requirement.
For example:
The FontTools PyPI distribution supports "extras", which are groups of additional dependencies that can be installed alongside FontTools via pip to enable specific features. For example:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that there is already a lack of total consistency in the docs regarding the capitalization of "FontTools" when it is used as the name of the project (versus, for example, referring to the modules). I see that this PR uses "FontTools" here, but uses "fontTools" in index.rst. Might as well be consistent within the PR, IMO.

Personally, I think starting with the capital F is the way to go, both because it more clearly differentiates between "project name" and "token" use cases, but also because of the project logo....

If there are other strong opinions, though, I'm happy to hear what they all are.


* `fs <https://pypi.org/pypi/fs>`__: (aka ``pyfilesystem2``) filesystem abstraction layer.
Package for reading and writing UFO source files. Dependencies include:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to spell out "Unified Font Object (UFO)" here, on first usage at this entry point. It's clear if the reader already knows what UFO is, but you are always going to get people who are diving in without a lot of prior background reading, because they've got some ultra-specific problem they're trying to fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should maybe add that I don't think it's necessary to always spell out UFO and other abbreviations, but this is intro material, dealing with step-one installation stuff, so I think the need is higher here....


To display the Unicode character names when dumping the ``cmap`` table
with ``ttx`` we use the ``unicodedata`` module in the Standard Library.
Displays Unicode character names when working with the `cmap` table dumps,with `ttx` we use the `unicodedata` module in the Standard Library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a space missing between "," and "with". I would also prefer to move towards "FontTools uses the unicodedata [...] over "we use [...]" but that might be a bigger discussion. Certainly it's not wrong to say "FontTools" instead of "we" though. Also, I would drop the "the" before cmap; pretty minor though.

(macOS platform only).

*Extra:* ``type1``
- `xattr <https://pypi.python.org/pypi/xattr>`__: Python wrapper for extended filesystem attributes, macOS only.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to disagree here, but I always worry about ambiguity when saying "macOS only" or the like unless it's totally spelled out. As in, are we saying that the module can only be installed on macOS? Or just that the xattrs are only found on macOS (different question how you'd be accessing those filesystems on another platform, though).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same "only"ness question on the CocoaPen bit below, I suppose, although I think mounting HFS/HFS+ disks is still possible on Linux, so the xattrs stuff might be usable there. I'd certainly be happy to learn for sure)

It requires one of the following packages in order to solve the so-called
"minimum weight perfect matching problem in bipartite graphs", or
the Assignment problem:
Module for resolving contour or component order between different masters. Requires following packages to solve 'minimum weight perfect matching problem in bipartite graphs':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a "the" before "following packages" ... or "one of the"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, if the Assignment Problem is well-defined enough to have a name, then it'd be ideal to drop a link about it where the name is mentioned. Wikipedia maybe?


*Extra:* ``ufo``
- `fs <https://pypi.org/pypi/fs>`__: (aka ``pyfilesystem2``) filesystem abstraction layer
- `enum34 <https://pypi.org/pypi/enum34>`__ backport for the built-in ``enum`` module (module for Python versions < 3.4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cut out the second "module" there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants